New Resource: aws_appautoscaling_scheduled_action #2231

atsushi-ishibashi · 2017-11-09T12:55:22Z

atsushi-ishibashi · 2017-11-11T07:03:07Z

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAppautoscalingScheduledAction_dynamo'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAppautoscalingScheduledAction_basic -timeout 120m
=== RUN   TestAccAwsAppautoscalingScheduledAction_basic
--- PASS: TestAccAwsAppautoscalingScheduledAction_basic (90.87s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	90.907s

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAppautoscalingScheduledAction_ECS'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAppautoscalingScheduledAction_ECS -timeout 120m
=== RUN   TestAccAwsAppautoscalingScheduledAction_ECS
--- PASS: TestAccAwsAppautoscalingScheduledAction_ECS (122.79s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	122.848s

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAppautoscalingScheduledAction_EMR'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAppautoscalingScheduledAction_EMR -timeout 120m
=== RUN   TestAccAwsAppautoscalingScheduledAction_EMR
--- PASS: TestAccAwsAppautoscalingScheduledAction_EMR (701.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	701.407s

make testacc TEST=./aws TESTARGS='-run=TestAccAwsAppautoscalingScheduledAction_SpotFleet'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAppautoscalingScheduledAction_SpotFleet -timeout 120m
=== RUN   TestAccAwsAppautoscalingScheduledAction_SpotFleet
--- PASS: TestAccAwsAppautoscalingScheduledAction_SpotFleet (330.93s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	330.970s

radeksimko

Thanks for this PR @atsushi-ishibashi
I left you some comments & questions, the most important one about support of updates and unique ID.

Let me know what you think.

radeksimko · 2017-12-02T02:37:43Z

aws/resource_aws_appautoscaling_scheduled_action.go

+					validDimensions := []string{"ecs:service:DesiredCount", "ec2:spot-fleet-request:TargetCapacity",
+						"elasticmapreduce:instancegroup:InstanceCount", "appstream:fleet:DesiredCapacity",
+						"dynamodb:table:ReadCapacityUnits", "dynamodb:table:WriteCapacityUnits",
+						"dynamodb:index:ReadCapacityUnits", "dynamodb:index:WriteCapacityUnits"}


While offline, plan-time validation is usually useful I'm afraid it could be counter-productive here as the list of valid dimensions may and will expand and it did recently. I think we'd play a constant catch-up with AWS here and rushing to cut a release just to support a new dimension just because of this strict validation.
Theoretically we could validate for something like [a-zA-Z]+:[a-zA-Z]+:[a-zA-Z]+ but frankly I'd just rather avoid the validation altogether here.

docs lists constant strings so I removed ValidateFunc.

radeksimko · 2017-12-02T02:39:02Z

aws/resource_aws_appautoscaling_scheduled_action.go

+				ForceNew: true,
+				ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
+					value := v.(string)
+					validServiceNamespaces := []string{"ecs", "elasticmapreduce", "ec2", "appstream", "dynamodb"}


As below - I'm afraid the list of valid service namespaces may/will grow and I'd like us to avoid the maintenance burden associated with it, if possible.

Ok, I removed ValidateFunc.

radeksimko · 2017-12-02T02:41:41Z

aws/resource_aws_appautoscaling_scheduled_action.go

+	}
+}
+
+func resourceAwsAppautoscalingScheduledActionCreate(d *schema.ResourceData, meta interface{}) error {


Per docs I think we should support updates of this resource and basically just rename this function to something like resourceAwsAppautoscalingScheduledActionCreateOrUpdate or resourceAwsAppautoscalingScheduledActionPut and reference it in Update too.

radeksimko · 2017-12-02T02:45:06Z

aws/resource_aws_appautoscaling_scheduled_action.go

+					return resource.RetryableError(err)
+				default:
+					return resource.NonRetryableError(err)
+				}


Q: Is this related to eventual consistency?

Regardless of the reason I think the above conditional starting on L153 can be simplified to something like this:

if err != nil { if isAWSErr(err, applicationautoscaling.ErrCodeObjectNotFoundException, "") { return resource.RetryableError(err) } return resource.NonRetryableError(err) }

I assumed that soon after creating dynamo table and on...
I simplified handling error.

radeksimko · 2017-12-02T02:46:35Z

aws/resource_aws_appautoscaling_scheduled_action.go

+		return nil
+	}
+	if len(resp.ScheduledActions) != 1 {
+		return fmt.Errorf("Number of Scheduled Action (%s) isn't one, got %d", saName, len(resp.ScheduledActions))


Grammar nitpick, do you mind changing this to Expected 1 scheduled action under %s, found %d?

radeksimko · 2017-12-02T02:50:23Z

aws/resource_aws_appautoscaling_scheduled_action_test.go

+      "Action": [
+        "ecs:DescribeServices",
+        "ecs:UpdateService",
+				"cloudwatch:DescribeAlarms"


Nitpick - extra whitespace here 👀

radeksimko · 2017-12-02T02:50:39Z

aws/resource_aws_appautoscaling_scheduled_action_test.go

+}
+
+resource "aws_ecs_cluster" "hoge" {
+	name = "tf-ecs-cluster-%s"


extra whitespace here 👀

radeksimko · 2017-12-02T02:50:57Z

aws/resource_aws_appautoscaling_scheduled_action_test.go

+}
+
+resource "aws_ecs_task_definition" "hoge" {
+  family = "foobar"


Do you mind using a random name here too, please?

radeksimko · 2017-12-02T02:52:48Z

website/docs/r/appautoscaling_scheduled_action.html.markdown

+The following arguments are supported:
+
+* `name` - (Required) The name of the scheduled action.
+* `service_namespace` - (Required) The namespace of the AWS service. Documentation can be found in the parameter at: [AWS Application Auto Scaling API Reference](https://docs.aws.amazon.com/ApplicationAutoScaling/latest/APIReference/API_PutScheduledAction.html#ApplicationAutoScaling-PutScheduledAction-request-ServiceNamespace) Valid Values: ecs | elasticmapreduce | ec2 | appstream | dynamodb


I would avoid documenting all valid values as that will become a maintenance burden as these expand. How about we just mention a few examples? The user can always IMO look up all valid values in the docs, right?

radeksimko · 2017-12-02T02:53:32Z

website/docs/r/appautoscaling_scheduled_action.html.markdown

+* `name` - (Required) The name of the scheduled action.
+* `service_namespace` - (Required) The namespace of the AWS service. Documentation can be found in the parameter at: [AWS Application Auto Scaling API Reference](https://docs.aws.amazon.com/ApplicationAutoScaling/latest/APIReference/API_PutScheduledAction.html#ApplicationAutoScaling-PutScheduledAction-request-ServiceNamespace) Valid Values: ecs | elasticmapreduce | ec2 | appstream | dynamodb
+* `resource_id` - (Required) The identifier of the resource associated with the scheduled action. Documentation can be found in the parameter at: [AWS Application Auto Scaling API Reference](https://docs.aws.amazon.com/ApplicationAutoScaling/latest/APIReference/API_PutScheduledAction.html#ApplicationAutoScaling-PutScheduledAction-request-ResourceId)
+* `scalable_dimension` - (Optional) The scalable dimension. Documentation can be found in the parameter at: [AWS Application Auto Scaling API Reference](https://docs.aws.amazon.com/ApplicationAutoScaling/latest/APIReference/API_PutScheduledAction.html#ApplicationAutoScaling-PutScheduledAction-request-ScalableDimension) Valid Values: ecs:service:DesiredCount | ec2:spot-fleet-request:TargetCapacity | elasticmapreduce:instancegroup:InstanceCount | appstream:fleet:DesiredCapacity | dynamodb:table:ReadCapacityUnits | dynamodb:table:WriteCapacityUnits | dynamodb:index:ReadCapacityUnits | dynamodb:index:WriteCapacityUnits


I would avoid documenting all valid values as that will become a maintenance burden as these expand. How about we just mention a few examples? The user can always IMO look up all valid values in the docs, right?

radeksimko

👍 🚢

* Make files * WIP * WIP * New Resource: aws_appautoscaling_scheduled_action * Reflect reviews * Used 3fields for id

ghost · 2020-04-10T17:18:34Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

radeksimko added the new-resource Introduces a new resource. label Nov 10, 2017

atsushi-ishibashi added 4 commits November 11, 2017 16:05

Make files

70493d4

WIP

490b467

WIP

9f1456e

New Resource: aws_appautoscaling_scheduled_action

99a586a

atsushi-ishibashi force-pushed the resource_aws_appautoscaling_scheduled_action branch from 502f7f8 to 99a586a Compare November 11, 2017 07:06

atsushi-ishibashi changed the title ~~[WIP]New Resource: aws_appautoscaling_scheduled_action~~ New Resource: aws_appautoscaling_scheduled_action Nov 11, 2017

radeksimko added the size/XL Managed by automation to categorize the size of a PR. label Nov 15, 2017

radeksimko suggested changes Dec 2, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 2, 2017

atsushi-ishibashi added 2 commits December 2, 2017 21:43

Reflect reviews

6a24fcf

Used 3fields for id

156e02a

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 4, 2017

radeksimko approved these changes Dec 4, 2017

View reviewed changes

radeksimko merged commit 01ff393 into hashicorp:master Dec 4, 2017

atsushi-ishibashi deleted the resource_aws_appautoscaling_scheduled_action branch December 13, 2017 15:16

atsushi-ishibashi mentioned this pull request Dec 20, 2017

Support: Schedule Action for Application Auto Scaling #2227

Closed

ghost locked and limited conversation to collaborators Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_appautoscaling_scheduled_action #2231

New Resource: aws_appautoscaling_scheduled_action #2231

atsushi-ishibashi commented Nov 9, 2017

atsushi-ishibashi commented Nov 11, 2017

radeksimko left a comment

radeksimko Dec 2, 2017

atsushi-ishibashi Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi Dec 2, 2017 •

edited

Loading

radeksimko Dec 2, 2017

atsushi-ishibashi Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi Dec 2, 2017 •

edited

Loading

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

radeksimko left a comment

ghost commented Apr 10, 2020

New Resource: aws_appautoscaling_scheduled_action #2231

New Resource: aws_appautoscaling_scheduled_action #2231

Conversation

atsushi-ishibashi commented Nov 9, 2017

atsushi-ishibashi commented Nov 11, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko left a comment

Choose a reason for hiding this comment

ghost commented Apr 10, 2020

atsushi-ishibashi Dec 2, 2017 •

edited

Loading

atsushi-ishibashi Dec 2, 2017 •

edited

Loading